Support the volatility config of function nodes #1414
Conversation
…nsure we get `EventCatcher` from package
dbt-snowflake/src/dbt/include/snowflake/macros/materializations/functions/scalar.sql
Outdated
Show resolved
Hide resolved
QMalcolm
force-pushed
the
qmalcolm--1345-begin-templating-volatility-for-scalar-functions
branch
from
36e15ee to
52aeb96
Compare
QMalcolm
had a problem deploying
to
dbt-snowflake
GitHub Actions
Error
QMalcolm
temporarily deployed
to
dbt-snowflake
GitHub Actions
Inactive
QMalcolm
had a problem deploying
to
dbt-bigquery
GitHub Actions
Failure
| {% if model.config.get('volatility') == 'deterministic' %} | ||
| IMMUTABLE | ||
| {% elif model.config.get('volatility') == 'stable' %} | ||
| STABLE | ||
| {% else %} | ||
| {# At this point, either they've set `non-deterministic` or they've set nothing. In either case, we default to VOLATILE #} | ||
| VOLATILE | ||
| {% endif %} |
There was a problem hiding this comment.
nit:
| {% if model.config.get('volatility') == 'deterministic' %} | |
| IMMUTABLE | |
| {% elif model.config.get('volatility') == 'stable' %} | |
| STABLE | |
| {% else %} | |
| {# At this point, either they've set `non-deterministic` or they've set nothing. In either case, we default to VOLATILE #} | |
| VOLATILE | |
| {% endif %} | |
| {% set volatility = model.config.get('volatility') %} | |
| {% if volatility == 'deterministic' %} | |
| IMMUTABLE | |
| {% elif volatility == 'stable' %} | |
| STABLE | |
| {% else %} | |
| {# At this point, either they've set `non-deterministic` or they've set nothing. In either case, we default to VOLATILE #} | |
| VOLATILE | |
| {% endif %} |
There was a problem hiding this comment.
also do we need to do any validation/warning if they have set something other than these options?
There was a problem hiding this comment.
(also in the future if we add another option do we need to remember to come back and raise a warning?)
There was a problem hiding this comment.
Core only accepts an Enum of possible volatility types deterministic, stable, and non-deterministic. So in order for a 4th option to come into existence, we'd first have to extend the enum in core. Additionally, have a hard time imagining any other volatility type being invented, but perhaps I'm being unimaginative 🤷🏻 Future proofing doesn't hurt anything.
There was a problem hiding this comment.
Okay I think I've now added logic to sufficiently handle this 🙂
| {% if model.config.get('volatility') == 'deterministic' %} | ||
| IMMUTABLE | ||
| {% elif model.config.get('volatility') == 'stable' %} | ||
| {% do exceptions.warn("`Stable` function volatility is not supported by Snowflake, and will be ignored") %} | ||
| {% elif model.config.get('volatility') == 'non-deterministic' %} | ||
| VOLATILE | ||
| {% endif %} |
There was a problem hiding this comment.
is this more future proof?
| {% if model.config.get('volatility') == 'deterministic' %} | |
| IMMUTABLE | |
| {% elif model.config.get('volatility') == 'stable' %} | |
| {% do exceptions.warn("`Stable` function volatility is not supported by Snowflake, and will be ignored") %} | |
| {% elif model.config.get('volatility') == 'non-deterministic' %} | |
| VOLATILE | |
| {% endif %} | |
| {% set volatility = model.config.get('volatility') %} | |
| {% if volatility == 'deterministic' %} | |
| IMMUTABLE | |
| {% elif model.config.get('volatility') == 'non-deterministic' %} | |
| VOLATILE | |
| {% elif volatility is not none %} | |
| {% do exceptions.warn("user passed unsupported volatility config value: " ~ volatility ~"; it will be ignored") %} | |
| {% endif %} |
There was a problem hiding this comment.
Okay I think I've now added logic to sufficiently handle this 🙂
| "Programming Language :: Python :: 3.12", | ||
| ] | ||
| dependencies = [ | ||
| "dbt-common>=1.34.0,<2.0", |
There was a problem hiding this comment.
can we let dbt-adapters set the range for dbt-common here?
There was a problem hiding this comment.
We can. My thought here was that we're specifying this range specifically for testing purposes (the EventCatcher is only used in testing). If putting it in dbt-adapters is more straight forward though, I'm good with doing so.
Technically it should be impossible for a new volatility to be created without our knowledge of it and for it to reach dbt-adapters macros. That is because `volatility` is defined as an `Enum` in core with only three possible values `deterministic`, `stable`, and `non-deterministic`. But if a new volatility is invented, and we start supporting setting it in core then we will want to handle it in dbt-adapters (by default as a warning) until we add _specific_ handling in dbt-adapters to support it.
QMalcolm
temporarily deployed
to
dbt-snowflake
GitHub Actions
Inactive
resolves #1345
Problem
dbt-core added the ability to set function node volatility in dbt-labs/dbt-core#12100. Thus in adapters we needed to begin interpolating the correct volatility value into the function materialization macros.
Solution
Checklist